Add app-wide AuthContext, Profile page, and navbar integration (Closes #47) #214
Conversation
… logout Add app-wide AuthContext and Profile page, wire into navbar and routing Changes: - src/context/AuthContext.tsx: new Auth provider that stores GitHub username and personal access token, persists to localStorage, exposes getOctokit() and logout(). - src/pages/Profile/Profile.tsx: new page to view/edit GitHub username and token. Saves to AuthContext and navigates back on save. - src/Routes/Router.tsx: added /profile route. - src/components/Navbar.tsx: updated header to show username when authenticated, with a dropdown containing View/Edit Profile and Logout. Mobile menu updated similarly. - src/main.tsx: app wrapped with AuthProvider so auth is available globally. - src/pages/Tracker/Tracker.tsx: switched to using AuthContext for username/token and getOctokit instead of local hook, so auth is centralized. Rationale: Centralized auth state enables consistent UX (navbar profile dropdown, logout, and profile editing) and avoids duplicating token/username state across pages. Storing credentials in localStorage provides a simple persistence layer; consider a more secure approach for production. Testing/Validation: - Ran a production build (vite build) locally; build succeeded. Notes and next steps: - Consider adding avatar fetch for the navbar, logout confirmation, and secure storage for PATs.
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe pull request introduces user authentication and profile management by creating an AuthContext, adding a Profile page route, and refactoring authentication state management across the application. Navbar now displays a user dropdown for authenticated users, Tracker consumes AuthContext instead of a hook, and all child components are wrapped with AuthProvider. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as UI Components
participant Auth as AuthContext
participant Storage as localStorage
User->>UI: Click View/Edit Profile
UI->>Auth: Access username & token
Auth->>Storage: Hydrate from localStorage
Storage-->>Auth: Return stored values
Auth-->>UI: Provide auth state
UI->>UI: Show Profile form pre-filled
User->>UI: Update credentials & Submit
UI->>Auth: Call setUsername & setToken
Auth->>Storage: Persist new values
Storage-->>Auth: Confirmed
Auth-->>UI: State updated
UI->>UI: Navigate to root
User->>UI: Click Logout
UI->>Auth: Call logout()
Auth->>Storage: Clear localStorage
Storage-->>Auth: Cleared
Auth-->>UI: State reset
UI->>UI: Show Login link
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Navbar.tsx (1)
169-198: Fix mobile menu UX inconsistency.The mobile menu shows the Login link unconditionally (lines 169-175) while also showing Profile/Logout buttons when authenticated (lines 176-198). This creates a confusing experience where authenticated users see both "Login" and "Logout" options. The desktop menu correctly shows either the user dropdown OR the login link.
Apply this diff to conditionally render the Login link:
<Link to="/contributors" className="block text-lg font-medium hover:text-gray-300 transition-all px-2 py-1 border border-transparent hover:border-gray-400 rounded" onClick={() => setIsOpen(false)} > Contributors </Link> - <Link - to="/login" - className="block text-lg font-medium hover:text-gray-300 transition-all px-2 py-1 border border-transparent hover:border-gray-400 rounded" - onClick={() => setIsOpen(false)} - > - Login - </Link> {auth && auth.username && ( <> <button onClick={() => { setIsOpen(false); navigate('/profile'); }} className="block text-left w-full text-lg font-medium hover:text-gray-300 transition-all px-2 py-1 border border-transparent hover:border-gray-400 rounded" > View / Edit Profile </button> <button onClick={() => { setIsOpen(false); auth.logout(); navigate('/login'); }} className="block text-left w-full text-lg font-medium hover:text-gray-300 transition-all px-2 py-1 border border-transparent hover:border-gray-400 rounded" > Logout </button> </> + ) : ( + <Link + to="/login" + className="block text-lg font-medium hover:text-gray-300 transition-all px-2 py-1 border border-transparent hover:border-gray-400 rounded" + onClick={() => setIsOpen(false)} + > + Login + </Link> )}
🧹 Nitpick comments (3)
src/context/AuthContext.tsx (3)
10-10: Improve type safety for getOctokit.The return type
any | nullreduces type safety. UseOctokit | nullfor better type checking and IDE support.Apply this diff:
type AuthContextType = { username: string; token: string; setUsername: (u: string) => void; setToken: (t: string) => void; logout: () => void; - getOctokit: () => any | null; + getOctokit: () => Octokit | null; };
56-63: Consider memoizing Octokit instance for performance.The
getOctokitfunction creates a new Octokit instance on every call. If components call this frequently (e.g., in render or effects), it could impact performance.Consider memoizing with
useMemo:+ const octokit = React.useMemo(() => { + if (!username || !token) return null; + try { + return new Octokit({ auth: token }); + } catch (e) { + return null; + } + }, [username, token]); + const getOctokit = () => { - if (!username || !token) return null; - try { - return new Octokit({ auth: token }); - } catch (e) { - return null; - } + return octokit; };
19-45: Security: Personal Access Tokens stored in localStorage.Storing GitHub PATs in localStorage exposes them to XSS attacks, as any JavaScript running on the page can access localStorage. While the PR description acknowledges this needs improvement for production, consider these alternatives:
- httpOnly cookies: More secure against XSS (requires backend support)
- sessionStorage: Cleared when tab closes (better than localStorage)
- Memory-only storage: Most secure but lost on refresh
For production, implement secure token storage with backend session management or encrypted cookies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Routes/Router.tsx(2 hunks)src/components/Navbar.tsx(3 hunks)src/context/AuthContext.tsx(1 hunks)src/main.tsx(1 hunks)src/pages/Profile/Profile.tsx(1 hunks)src/pages/Tracker/Tracker.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/pages/Profile/Profile.tsx (1)
src/context/AuthContext.tsx (1)
AuthContext(13-13)
src/pages/Tracker/Tracker.tsx (1)
src/context/AuthContext.tsx (1)
AuthContext(13-13)
src/components/Navbar.tsx (2)
src/context/ThemeContext.tsx (1)
ThemeContext(10-10)src/context/AuthContext.tsx (1)
AuthContext(13-13)
🪛 Biome (2.1.2)
src/pages/Profile/Profile.tsx
[error] 13-13: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 14-14: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (3)
src/Routes/Router.tsx (1)
9-9: LGTM!The Profile route is properly imported and integrated consistently with the existing routing structure.
Also applies to: 22-22
src/main.tsx (1)
7-7: LGTM!The AuthProvider is correctly positioned in the component tree. Components that consume AuthContext and use routing hooks (like Navbar and Profile) are rendered inside both the AuthProvider and BrowserRouter, ensuring they have access to both contexts.
Also applies to: 12-16
src/pages/Profile/Profile.tsx (1)
37-45: Verify: Token field is not required.The Personal Access Token field lacks the
requiredattribute (unlike the username field on line 33), allowing users to save an empty token. If this is intentional for clearing tokens, consider adding a confirmation or explicit "Clear Token" action for better UX.
| const auth = useContext(AuthContext); | ||
| const navigate = useNavigate(); | ||
|
|
||
| if (!auth) return null; | ||
|
|
||
| const { username, token, setUsername, setToken } = auth; | ||
|
|
||
| const [localUsername, setLocalUsername] = useState(username || ""); | ||
| const [localToken, setLocalToken] = useState(token || ""); |
There was a problem hiding this comment.
Critical: Fix hook ordering to comply with Rules of Hooks.
The useState hooks on lines 13-14 are called after the early return on line 9, violating React's Rules of Hooks. Hooks must be called unconditionally in the same order on every render.
Apply this diff to fix the hook ordering:
const Profile: React.FC = () => {
const auth = useContext(AuthContext);
const navigate = useNavigate();
+ const [localUsername, setLocalUsername] = useState("");
+ const [localToken, setLocalToken] = useState("");
if (!auth) return null;
const { username, token, setUsername, setToken } = auth;
-
- const [localUsername, setLocalUsername] = useState(username || "");
- const [localToken, setLocalToken] = useState(token || "");
+
+ // Sync with auth context values on mount/change
+ React.useEffect(() => {
+ setLocalUsername(username || "");
+ setLocalToken(token || "");
+ }, [username, token]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const auth = useContext(AuthContext); | |
| const navigate = useNavigate(); | |
| if (!auth) return null; | |
| const { username, token, setUsername, setToken } = auth; | |
| const [localUsername, setLocalUsername] = useState(username || ""); | |
| const [localToken, setLocalToken] = useState(token || ""); | |
| const auth = useContext(AuthContext); | |
| const navigate = useNavigate(); | |
| const [localUsername, setLocalUsername] = useState(""); | |
| const [localToken, setLocalToken] = useState(""); | |
| if (!auth) return null; | |
| const { username, token, setUsername, setToken } = auth; | |
| // Sync with auth context values on mount/change | |
| React.useEffect(() => { | |
| setLocalUsername(username || ""); | |
| setLocalToken(token || ""); | |
| }, [username, token]); |
🧰 Tools
🪛 Biome (2.1.2)
[error] 13-13: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 14-14: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/pages/Profile/Profile.tsx around lines 6 to 14, the useState hooks are
called after an early return (if (!auth) return null), violating the Rules of
Hooks; move the hooks to execute unconditionally before any early returns: call
useContext(AuthContext), then declare the useState hooks (initializing with
auth?.username and auth?.token or empty strings) immediately, then perform the
guard/early-return based on auth (or render a loading/null) and finally
destructure setters from auth if needed; ensure hook order is preserved on every
render.
| const auth = useContext(AuthContext); | ||
|
|
||
| const username = auth?.username || ""; | ||
| const setUsername = auth?.setUsername || (() => {}); | ||
| const token = auth?.token || ""; | ||
| const setToken = auth?.setToken || (() => {}); | ||
| const authError = ""; | ||
| const getOctokit = auth?.getOctokit || (() => null); |
There was a problem hiding this comment.
Major issue: Authentication error handling removed.
The authError is hardcoded to an empty string (line 59), removing any authentication error feedback that may have been present in the previous useGitHubAuth hook implementation. Users won't see errors related to invalid credentials, API rate limits, or token issues.
Consider adding error state to AuthContext or handling authentication errors within the Tracker component:
const auth = useContext(AuthContext);
const username = auth?.username || "";
const setUsername = auth?.setUsername || (() => {});
const token = auth?.token || "";
const setToken = auth?.setToken || (() => {});
- const authError = "";
+ const [authError, setAuthError] = useState("");
const getOctokit = auth?.getOctokit || (() => null);Then wrap the getOctokit call or data fetching with try-catch to set authError when authentication fails.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/pages/Tracker/Tracker.tsx around lines 53 to 60, authError is hardcoded
to an empty string which strips out authentication error handling; restore a
real error state by either reading authError from AuthContext (add it to the
context if missing) or creating a local state const [authError, setAuthError] =
useState<string>("") and remove the hardcoded value, then wrap getOctokit calls
and any GitHub API data fetching in try/catch blocks that call setAuthError with
the caught error message (and clear it on success); ensure any UI that showed
authentication errors uses this error state so users see credential/API/token
issues.
nishanth-kj
changed the title
1 participant
Related Issue
Description
Add a centralized AuthContext that stores GitHub username and personal access token (PAT), persists them to
localStorage, and exposesgetOctokit()andlogout(). Add a Profile page to view and edit credentials. Wire authentication into the navbar and routing so the UI reflects authenticated state and provides a profile dropdown and logout.Changes
AuthProviderpersisting username and token, exposinggetOctokit()andlogout().AuthContextand navigates back on save./profileroute.AuthProvider.AuthContextfor username/token andgetOctokit().Rationale
Centralizes auth state to avoid duplicated token/username management and enable consistent UX (profile dropdown, logout, profile editing).
localStorageprovides simple persistence for now. Consider more secure storage for production.How Has This Been Tested?
vite buildsucceeded.AuthContextfor API calls.Notes / Next Steps
localStoragewith secure storage for PATs in production.Screenshots
(If needed, add screenshots showing Profile page and updated Navbar.)
Type of Change
Release Notes
AuthContextwithgetOctokit()andlogout().